Code Review: Qwen OAuth Authentication

Thanks for this contribution! The overall implementation looks solid. Here are my findings:

✅ What's Done Well

1. PKCE Implementation - Uses S256 challenge method, compliant with RFC 7636
2. Auto Token Refresh - 5-minute buffer design is reasonable
3. Error Handling - Comprehensive OAuth error state coverage (authorization_pending, slow_down, expired_token, access_denied)
4. Documentation - Both README.md and README.zh.md are updated

🔴 Critical Issues

1. helpers.go:204-220 - Forcefully Overwrites User Configuration

```go
if isQwenModel(appCfg.ModelList[i].Model) {
appCfg.ModelList[i].Model = "qwen-portal/coder-model" // Forced overwrite!
appCfg.ModelList[i].APIBase = "https://portal.qwen.ai/v1"
appCfg.ModelList[i].APIKey = "qwen-oauth"
appCfg.ModelList[i].AuthMethod = "oauth"
found = true
break
}
```

Problem: If a user has already configured a custom qwen model (e.g., `qwen/qwen-max`), it will be forcibly changed to `coder-model` after login.

Suggestion: Only update `AuthMethod` and credential-related fields, preserve the original model configuration.

2. helpers.go:224 - Silently Modifies Default Model

```go
appCfg.Agents.Defaults.ModelName = "qwen-coder"
```

Problem: After login, the user's default model is automatically changed to qwen-coder without consent. This is too aggressive.

🟡 Medium Issues

3. qwen_oauth.go:354, 384, 528 - HTTP Client Created Repeatedly

```go
client := &http.Client{Timeout: 15 * time.Second} // New client every request
```

Suggestion: Reuse HTTP client, or use `sync.Once` for lazy initialization.

4. qwen_oauth.go:577-580 - Refresh Failure Silently Ignored

```go
newCred, refreshErr := RefreshQwenCredentials(cred)
if refreshErr == nil {
_ = SetCredential("qwen", newCred)
return newCred.AccessToken, nil
}
// Refresh failed but token may still be valid; fall through
```

Problem: No logging when refresh fails, users won't know their token is about to expire.

5. qwen_provider.go:716-717 - Repeated TrimPrefix Calls

```go
model = strings.TrimPrefix(model, "qwen-oauth/")
model = strings.TrimPrefix(model, "qwen/")
```

Could be simplified or extracted to a utility function.

6. Code Duplication - `convertMessagesForQwen` and `convertToolsForQwen`

These functions are nearly identical to implementations in other providers (OpenAI, DeepSeek). Consider extracting to a common module.

🟢 Minor Issues

7. qwen_oauth.go:589-595 - Unused Parameter

```go
func printSimpleQRHint(verifyURL string) {
// ...
_ = verifyURL // Parameter is completely unused
}
```

If `verifyURL` isn't used, don't pass it as a parameter.

8. helpers.go:181-186 - Incomplete `isQwenModel` Check

```go
func isQwenModel(model string) bool {
return model == "qwen" ||
model == "qwen-oauth" ||
strings.HasPrefix(model, "qwen/") ||
strings.HasPrefix(model, "qwen-oauth/")
}
```

Missing the `qwen-portal` alias which is supported in `factory_provider.go`.

🔧 Suggested Fixes

```go
// helpers.go - Don't overwrite user configuration
if isQwenModel(appCfg.ModelList[i].Model) {
// Only update auth-related fields, preserve original model name
appCfg.ModelList[i].AuthMethod = "oauth"
if appCfg.ModelList[i].APIBase == "" {
appCfg.ModelList[i].APIBase = "https://portal.qwen.ai/v1"
}
found = true
break
}

// Remove this line - don't change default model without consent
// appCfg.Agents.Defaults.ModelName = "qwen-coder"
```

📊 Summary

Recommendation: Fix the two critical issues (forced configuration overwrite) before merging. Other issues can be addressed in follow-up PRs.

Review: Qwen OAuth Authentication Implementation

Thanks for this comprehensive feature addition! The implementation is well-structured with good test coverage (32 test cases). Here are my observations:

✅ Strengths

1. Security: PKCE (RFC 7636) implementation is correct with 32-byte random verifier and S256 challenge
2. Error Handling: Proper handling of OAuth states (authorization_pending, slow_down, expired_token, access_denied)
3. Token Management: Auto-refresh with 5-minute buffer is a sensible design choice
4. Test Coverage: Comprehensive unit tests for both OAuth flow and provider logic
5. Documentation: Complete bilingual documentation (EN/CN)

⚠️ Suggestions for Improvement

1. pollQwenToken is not cancellable (Medium Priority)

Location: pkg/auth/qwen_oauth.go:413-476

for time.Now().Before(deadline) {
    time.Sleep(pollInterval)  // Cannot be cancelled by user
    // ...
}

If a user wants to cancel the login (Ctrl+C), the polling continues until timeout.

Suggested fix:

func pollQwenToken(ctx context.Context, deviceCode, verifier string, interval, expiresIn int) (*qwenTokenResponse, error) {
    // ...
    for time.Now().Before(deadline) {
        select {
        case <-time.After(pollInterval):
        case <-ctx.Done():
            return nil, ctx.Err()
        }
        // ...
    }
}

2. printSimpleQRHint ignores its parameter (Low Priority)

Location: pkg/auth/qwen_oauth.go:625-632

func printSimpleQRHint(verifyURL string) {
    // ...
    _ = verifyURL  // Parameter unused
}

Consider adding a TODO comment if this is a placeholder for actual QR code rendering, or remove the parameter if intentionally unused.

3. Protocol prefix inconsistency (Low Priority)

Location: cmd/picoclaw/internal/auth/helpers.go:455-460

appCfg.ModelList[i].Model = "qwen-portal/coder-model"

The documentation primarily describes qwen-oauth. Consider using the canonical name consistently:

appCfg.ModelList[i].Model = "qwen-oauth/coder-model"

📝 Summary

Recommendation: Address the cancellable polling issue, then this PR is ready to merge. Great work!